-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DEVEXP-381: Flexible enums for regions #54
Conversation
…Client-parameters
@@ -86,13 +87,14 @@ export class ConversationDomainApi implements Api { | |||
return this.client; | |||
} | |||
|
|||
private buildHostname(region: ConversationRegion) { | |||
private buildHostname(region: ConversationRegionFlexible) { | |||
const formattedRegion = region === '' ? region : `${region}.`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it come from myself, but thank you to small function usage here: my first reading of this line was cause of reflexion to understand logic and thinking there was a issue:
- testing against an empty value to use the testing value (the empty one)
- if not empty: use it too....
🤔
Then I realized the final dot for second case; then use to format a string
Just a personal thought I think I would have written logic as
const formattedRegion = region !== '' ? `${region}.` '';
Which could highlight in a better way the logic and use case.
Again just a personal thought and not a blocker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it would be more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed for all 4 APIs that use regions
console.error(`No region exist for the value '${value}'`); | ||
return undefined; | ||
} | ||
export type SmsRegionFlexible = SmsRegion | string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By inverting the point of view; could the type being more user friendly ?
I mean:
export enum SupportedSmsRegion {
UNITED_STATES = 'us',
EUROPE = 'eu',
BRAZIL = 'br',
CANADA = 'ca',
AUSTRALIA = 'au'
}
export type SmsRegion = SupportedSmsRegion | string;
So when function prototype or variable usage will reference the type, it will be named SmsRegion
.
Seems to be more "user friendly" than a SmsRegionFlexible
reference and usage.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be more understandable indeed. The issue being that with this implementation, I can't do SmsRegion.EUROPE as SmsRegion refers to a Type, but would be used as a value
A solution would be to trick the user and define on top of the enum a new object SmsRegion such as
export const SmsRegion = {
...SupportedSmsRegion,
};
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As usual, we should have end user and SDK usage in mind first.
so if a SmsRegion
type is identified as the best way to reach this objective, if by implementing it as you described in your comment above: it is very fine
Then, using same pattern for Conversation, if SDK usage enable to store a variable/use a parameter type named ConversationRegion
and reference ConversationRegion.UNITED_STATES
or use us
directly : it should provide a better DX (IMO)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we can do that. Keep in mind the trick I mentioned above: ConversationRegion.UNITED_STATES will not reference the enum, but an object of which we read the value associated to the key UNITED_STATES
😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
No description provided.